Create conversion graph #1627

theunrepentantgeek · 2021-07-07T03:30:59Z

What this PR does / why we need it:

Adds a pipeline stage that computes the conversion graph required for our storage versioning.

Prerequisites

Merge PR Convert Direction from enum into a discriminated union #1618
Merge PR Move Property Assignment Function into the functions package #1622
Merge PR Introduce Pipeline State #1644
Rebase on master

How does this PR make you feel:

If applicable:

this PR contains tests

babbageclunk

This looks great mostly but I think the graph creation stage needs to deal with types from more than one group. (Although I could be wrong!)

babbageclunk · 2021-07-12T06:01:13Z

hack/generator/pkg/codegen/pipeline/create_conversion_graph.go

+			for i, ref := range sortedApiReferences {
+				if i == 0 || !ref.IsPreview() {
+					continue
+				}
+
+				conversionGraph.AddLink(sortedStorageReferences[i], sortedStorageReferences[i-1])
+			}


I don't understand this part. Don't the references need to be split by group before you can do this? Otherwise you'll get the earliest version of one group pointing at the latest version of the previous group - something like microsoft.network/v20150101 pointing to microsoft.compute/v20210701. In my head each stage runs with the full set of loaded types. Is that right? Or is there something structural that means that the stage only runs with (all versions of) the types from one group?

Yes, you're perfectly correct - I transplanted this code from #1533 and put it in the wrong place. 😲

I'll relocate it into GroupConversionGraph and add some tests to ensure it behaves correctly.

babbageclunk · 2021-07-12T06:10:25Z

hack/generator/pkg/codegen/pipeline/testdata/TestCreateStorageTypes-v20211231.golden

@@ -3,10 +3,20 @@
 // Licensed under the MIT license.
 package v20211231


Should the package have changed? Or is the destination package an input to the code that this test is checking?

I'll double check this after moving the graph creation code; when I reviewed these changes prior to creating the PR I was sure they were correct, but I'm no longer convinced.

They have no longer changed - I fixed a couple of bugs in the test. Good catch.

babbageclunk · 2021-07-12T06:11:57Z

...nerator/pkg/codegen/pipeline/testdata/TestInjectPropertyAssignmentFunctions-v20200101.golden

 type Person struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata,omitempty"`
 	Spec              Person_Spec   `json:"spec,omitempty"`
 	Status            Person_Status `json:"status,omitempty"`
 }

-// AssignPropertiesFromPerson populates our Person from the provided source Person


Why did these go away? Aren't they still needed for conversion to storage versions?

These have no longer changed either.

codecov-commenter · 2021-07-14T05:09:36Z

Codecov Report

Merging #1627 (9472d0b) into master (c1e1902) will increase coverage by 0.17%.
The diff coverage is 90.98%.

@@            Coverage Diff             @@
##           master    #1627      +/-   ##
==========================================
+ Coverage   67.04%   67.22%   +0.17%     
==========================================
  Files         205      208       +3     
  Lines       14898    14979      +81     
==========================================
+ Hits         9989    10069      +80     
+ Misses       4152     4151       -1     
- Partials      757      759       +2

Impacted Files	Coverage Δ
...ck/generator/pkg/codegen/storage/type_converter.go	`71.66% <83.33%> (ø)`
...or/pkg/codegen/storage/conversion_graph_builder.go	`86.20% <86.20%> (ø)`
...or/pkg/codegen/pipeline/create_conversion_graph.go	`86.66% <86.66%> (ø)`
.../generator/pkg/codegen/storage/conversion_graph.go	`81.81% <88.88%> (-7.66%)`	⬇️
.../codegen/storage/group_conversion_graph_builder.go	`94.28% <94.28%> (ø)`
hack/generator/pkg/astmodel/package_reference.go	`90.99% <100.00%> (+3.02%)`	⬆️
hack/generator/pkg/codegen/code_generator.go	`89.03% <100.00%> (+0.07%)`	⬆️
...rator/pkg/codegen/pipeline/create_storage_types.go	`91.66% <100.00%> (-0.34%)`	⬇️
...g/codegen/pipeline/inject_original_gvk_function.go	`76.19% <100.00%> (ø)`
...degen/pipeline/inject_original_version_function.go	`88.23% <100.00%> (ø)`
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1e1902...9472d0b. Read the comment docs.

theunrepentantgeek · 2021-07-18T19:48:51Z

/azp run

azure-pipelines · 2021-07-18T19:49:05Z

Azure Pipelines successfully started running 1 pipeline(s).

babbageclunk

Looks great - some minor comments.

babbageclunk · 2021-07-21T00:25:38Z

hack/generator/pkg/codegen/pipeline/create_conversion_graph_test.go

+	person2022 := test.CreateSpec(test.Pkg2022, "Person", test.FullNameProperty, test.KnownAsProperty, test.FamilyNameProperty)
+
+	types := make(astmodel.Types)
+	types.AddAll(person2020, person2021, person2022)


Is it worth having some unrelated types in here to make sure there aren't any links created between Person and OtherType?

Noted for a followup PR.

babbageclunk · 2021-07-21T00:34:49Z

hack/generator/pkg/codegen/pipeline/inject_property_assignment_functions_test.go

+	createConversionGraphStage := CreateConversionGraph()
+	state, err := createConversionGraphStage.Run(context.TODO(), state)
+	g.Expect(err).To(Succeed())


This might be better done as just constructing the intended conversion graph, rather than calling the stage to build it. The current way makes it an integration test for the interaction between the three stages (CreateConversionGraph, CreateStorageTypes and InjectFunctions) . That's good for making sure that they all work together, but makes it hard to pinpoint the problem if it fails. I can see that it would be hard to create the storage types manually, but I think the graph should be reasonably easy to build by hand?

If a problem occurs with an earlier stage, the tests on that stage should flag it. I tried manually creating the required state but the amount of code required was prohibitive and it didn't seem to provide any value - in fact, it obscured what the test was doing. I have later changes (different PRs) to simplify the tests further.

hack/generator/pkg/codegen/storage/group_conversion_graph_test.go

Co-authored-by: Christian Muirhead <christian.muirhead@microsoft.com>

theunrepentantgeek added this to the codegen-alpha-1 milestone Jul 7, 2021

theunrepentantgeek self-assigned this Jul 7, 2021

theunrepentantgeek requested review from babbageclunk, davefellows, matthchr and Porges as code owners July 7, 2021 03:31

theunrepentantgeek force-pushed the feature/create-conversion-graph branch from 33880e9 to 4c39ccf Compare July 7, 2021 04:24

theunrepentantgeek mentioned this pull request Jul 7, 2021

Implement the Convertible interface on non-hub resources #1628

Merged

6 tasks

theunrepentantgeek force-pushed the feature/move-property-assignment-function branch from 1c75733 to fa329cf Compare July 8, 2021 21:03

Base automatically changed from feature/move-property-assignment-function to master July 8, 2021 21:27

theunrepentantgeek force-pushed the feature/create-conversion-graph branch 2 times, most recently from b86d27f to cf5198b Compare July 8, 2021 22:34

Porges approved these changes Jul 8, 2021

View reviewed changes

theunrepentantgeek force-pushed the feature/create-conversion-graph branch from b2b1ec5 to 60d27fd Compare July 11, 2021 23:48

theunrepentantgeek mentioned this pull request Jul 12, 2021

Eliminate most use of hard coded strings for pipeline stage names #1640

Merged

4 tasks

babbageclunk requested changes Jul 12, 2021

View reviewed changes

theunrepentantgeek force-pushed the feature/create-conversion-graph branch from df32912 to e9e04cf Compare July 14, 2021 02:50

theunrepentantgeek mentioned this pull request Jul 14, 2021

Implement conversions for Specs and Statuses #1649

Merged

5 tasks

theunrepentantgeek added 10 commits July 18, 2021 11:51

Add FindHubTypeName() to ConversionGraph and test

43179f9

Move shared test values into test package

196b165

Add CreateConversionGraph() pipeline stage

3218606

Integrate into the pipeline

41193e6

Code gardening

c4f42b6

Fix stage naming

6ca3b81

Fix bug in IsPreview() affecting tests

58dcf5e

Implement GroupConversionGraphBuilder and test

c2202a5

Implement ConversionGraphBuilder and test

5756d23

Convert pipeline stage

85de224

theunrepentantgeek added 2 commits July 18, 2021 11:51

Update tests

b58002e

Tweaks for CI

9a876ce

theunrepentantgeek force-pushed the feature/create-conversion-graph branch from 3520a61 to 9a876ce Compare July 17, 2021 23:51

Update golden file

48fb6c8

theunrepentantgeek added 3 commits July 21, 2021 11:24

Merge branch 'master' into feature/create-conversion-graph

9acaa7f

Attempt downgrade of controller-runtime

cca7dd8

Attempt downgrade of controller-runtime, take II

d28068b

babbageclunk approved these changes Jul 21, 2021

View reviewed changes

Update as per PR comment

9472d0b

Co-authored-by: Christian Muirhead <christian.muirhead@microsoft.com>

theunrepentantgeek merged commit b7ccaba into master Jul 21, 2021

theunrepentantgeek deleted the feature/create-conversion-graph branch July 21, 2021 02:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create conversion graph #1627

Create conversion graph #1627

theunrepentantgeek commented Jul 7, 2021 •

edited

Loading

babbageclunk left a comment

babbageclunk Jul 12, 2021

theunrepentantgeek Jul 12, 2021 •

edited

Loading

babbageclunk Jul 12, 2021

theunrepentantgeek Jul 12, 2021

theunrepentantgeek Jul 14, 2021

babbageclunk Jul 12, 2021

theunrepentantgeek Jul 14, 2021

codecov-commenter commented Jul 14, 2021 •

edited

Loading

theunrepentantgeek commented Jul 18, 2021

azure-pipelines bot commented Jul 18, 2021

babbageclunk left a comment

babbageclunk Jul 21, 2021

theunrepentantgeek Jul 21, 2021

babbageclunk Jul 21, 2021

theunrepentantgeek Jul 21, 2021

		@@ -3,10 +3,20 @@
		// Licensed under the MIT license.
		package v20211231

Create conversion graph #1627

Create conversion graph #1627

Conversation

theunrepentantgeek commented Jul 7, 2021 • edited Loading

babbageclunk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theunrepentantgeek Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 14, 2021 • edited Loading

Codecov Report

theunrepentantgeek commented Jul 18, 2021

azure-pipelines bot commented Jul 18, 2021

babbageclunk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theunrepentantgeek commented Jul 7, 2021 •

edited

Loading

theunrepentantgeek Jul 12, 2021 •

edited

Loading

codecov-commenter commented Jul 14, 2021 •

edited

Loading